feat(observable): coercion, typed observable #623

bigopon · 2017-09-26T14:18:06Z

This PR

adds coerce support for decorator observable
adds ability to create custom observable with fluent syntax: @observable.custom
provide 4 base custom observables: @observable.date, @observable.number, @observable.string, @observable.boolean

Usage:

With normal syntax

class MyViewModel {
  @observable({ coerce: 'number' }) numProp
  @observable({ coerce: 'boolean' }) boolProp
  @observable({ coerce: val => convertValue(val) }) customCoerceProp
}

Using metadata

import {metadata} from 'aurelia-metadata';

class MyViewModel {
  @observable
  @Reflect.metadata(metadata.propertyType, Number)
  num
}

var instance = new MyViewModel();
instance.num = '4';
instance.num; // <====== 4

TypeScript users can achieve above result (metadata) by simpler code:

class MyViewModel {
  @observable num: number
}

var instance = new MyViewModel();
instance.num = '4';
instance.num; // <===== 4 , with compile error though

Instruction for the compiler to emit decorator metadata TypeScript decorator doc

All coerce type will be resolved to a string, which then is used to get the converter function in coerceFunctions export of this module. So, to extend or modify basic implementations:

import {coerceFunctions} from 'aurelia-binding';

// Modify built in
coerceFunctions.string = function(a) {
  return a === null || a === undefined ? '' : a.toString();
}

// Extend
coerceFunctions.point = function(a) {
  return a.split(' ').map(parseFloat).slice(0, 2);
}

// Usage of 'point' coerces defined above:
class MyLine {
  @observable({ coerce: 'point' }) point1
  @observable({ coerce: 'point' }) point2
}

For TS users or JS users who want to use metadata, to extend coerce mapping:

import {
  createTypedObservable
} from 'aurelia-binding';

// use static class method
class Point {
  static coerce(value) {
    return new Point(value);
  }
}
mapCoerceFunction(Point, 'point');

// or just pass a 3rd parameter, fallback to static coerce method when 3rd param omitted:
mapCoerceFunction(Point, 'point', val => new Point(val));

// decorator usage:
// TypeScript
class MyLine {
  @observable point1: Point
  @observable point2: Point
}
// JavaScript
class MyLine {
  @observable
  @Reflect.metatata('design:type', Point)
  point1

  // or like this
  @observable({ coerce: 'point' })
  point2
}

With fluent syntax

class MyLine {
  @observable.number x1
  @observable.number() y1

  @observable.number() x2
  @observable.number y2
}

var line = new MyLine();

line.x1 = '15'
line.x1; // <======= 15

To built your own fluent syntax observable:

import {
  coerceFunctions,
  createTypedObservable
} from 'aurelia-binding'

coerceFunctions.point = function(value) {
  return value.split(' ').map(parseFloat);
}
createTypedObservable('point');

// usage:
class MyLine {
  @observable.point point1;
  @observable.point point2;
}

jdanyow · 2017-09-30T17:03:25Z

Very nice 💪- have you had a chance to try this out in data-binding scenarios? I imagine there must be some gotchas when you attempt to use this with two-way binding. What are the limitations that folks should know about? What are some good example use-cases we can put in the docs for this feature?

bigopon · 2017-10-01T01:10:33Z

@jdanyow For two way data binding on text field, it is normal with string, ok-ish with number, and broken for any other type (date, boolean).

I feel like there's a lot of gotchas, but cannot really describe them. I've prepared a gist at https://gist.run/?id=7d59c430a382b3423a13d7f760eb2c00 to demo common cased for typed observable and bindable.

** There is a small modification for date coerce function

date: function(val) {
  // instead of return new Date(val);
  if (val === null || val === undefined) {
    return null;
  }
  const d = new Date(val);
  const t = d.getTime(); // to deal with invalid date
  return t === t ? d : null;
}

Edit 1: Another concerns is @observable self subscriber behaves in different way than @bindable, it's executed immediately for observable but delayed-ly for bindable. This could be related: #594

Edit 2: I can start putting together a list of gotchas here

Gotchas

Using typed @observable / @bindable between two component:

Avoid typed observable / bindable declared in both sides of the binding if it's not primitive type, if you convert the incoming value to new object every time, as syncing between two sides of binding will happen indefinitely. Example:
- <App/> date [observable.date] <-> <DatePicker/> date [bindable.date]: When date property on DatePicker changes, App date will be notified and convert incoming date object, called d1, from DatePicker to a new date object, called d2 to assign to its date property. Binding system will attempt to check if both sides of the binding are synced by doing d2 === d1, which will never happen, thus triggers syncing indefinitely.
- <App/> amount [number] <-> <NumberField/> value [bindable.number]: When value property on NumberField changes, App amount will be notified and assigned new number value. Binding system will attempt to check if both sides of the binding are synced by doing n2 === n1, which is true, and stops.

Binding to text input:

Avoid any typed observable / bindable beside number, as <input /> element coerces everything to string, and it's quite hard to find a good solution to convert back and forth precisely for anything type beside number.
For number bindable / observable, changes from <input /> element with very big value, for example: "2222222222222222222222222222222222222222", will be converted to 2.2222222222222223e+39 instead of its original format, thus will have issue with syncing between the view model and the view. It's should be converted manually.

@jdanyow For gotacha 1, maybe we can employ a validator to see if a value is valid to be set. Could be related aurelia/templating#551

What do you think ?

EisenbergEffect · 2017-10-16T06:13:33Z

@jdanyow Can you review this?
@bigopon Can you confirm that all syntax is the same for the typed bindables as well?

bigopon · 2017-10-16T12:38:05Z

@jdanyow @EisenbergEffect
I just discovered a seemingly problematic behavior when using property type with metadata: We are defining the metadata on the prototype, not the constructor. Which will affect serialization scenarios, and probably inheritance too. Example Gist (Check the console, you will see __metadata__ on the prototype of App)
https://gist.run/?id=7d59c430a382b3423a13d7f760eb2c00

Explanation:

When doing:

javascript using Reflect.metadata:

class MyClass {
  @Reflect.metadata('design:Type', Number) age
}

or

typescript: emit metadata:

class MyClass {
  age: number
}

We are doing basically this:

Reflect.defineMetadata('design:Type', Number, MyClass.prototype, 'age');

The work around is probably the decorators have too loop over the prototype chain and find the corresponding metadata for the properties. What do you think @jdanyow @EisenbergEffect

nashwaan · 2017-10-19T05:41:26Z

Wow @bigopon .
This feature will solve and simplify many scenarios.

💪💪💪💪💪

bigopon · 2017-10-19T10:30:38Z

@jdanyow @EisenbergEffect sorry false alarm. Information about coercion for inheritance in both @observable and @bindable are already carried to derived class (slightly different ways) so it will work fine. Added tests for inheritance for @observable in this PR though.

One thing should be discussed is how we want the API for turn on / off the flag to auto pickup type metadata. Two options atm:

let _usePropertyType = false;
export function usePropertyType(shouldUsePropertyType: boolean) {
  _usePropertyType = shouldUsePropertyType;
}

// OR
observable.usePropertyType = function(shouldUsePropertyType: boolean) {
  _usePropertyType = shouldUsePropertyType;
}

Working on bindable now and should be able to do some documentation soon.

atsu85 · 2017-10-19T19:52:59Z

src/coerce-functions.js

+    }
+    const d = new Date(val);
+    const t = d.getTime(); // to deal with invalid date
+    return t === t ? d : null;


is this t === t an optimization compared to !isNaN(t), or why do you use this weird syntax, that only makes sense in JavaScript?

Yes it is used to specifically deal with NaN value. As NaN === NaN is false.

What a spec

But does it perform better than using isNaN?

Thats interesting question. Normally i would say yes but V8 is smart and I believe isNaN could be faster ( from another v8 engineer tweet i saw different between o === o and Object.is(o, o) is 2 us

@bigopon

As NaN === NaN is false.

Yeah i know, right after figuring that out I started using TsLint ;)

My comment+question was because of readability & preformance. IMHO if t === t is not preforming (much) better than !isNaN(t), then i'd vote for better readability :)

I see. Cant commit on phone 😁 Will change

@atsu85 thanks for the suggestion. I just keep doing those things 😄

bigopon · 2017-10-20T13:30:16Z

Demo for this PR + typed bindable is at http://aurelia-typed-observable-bindable.surge.sh/

IE 11 compatible so we can test the binding scenarios for all supported browsers.

bigopon · 2017-12-10T23:07:39Z

@EisenbergEffect This PR is base for aurelia/templating#558, and is waiting for some review from @jdanyow . Beside one thing that is I can no longer break it when setting coerce (and create new object to assign) on both side of two way binding, which is quite unexpected.

If this is good to merge then the other can be merged straight away I believe

EisenbergEffect · 2017-12-10T23:14:28Z

@bigopon I didn't quite follow that last bit. Are you saying that two-way patterns work now as well?

jdanyow · 2017-12-11T00:15:43Z

@bigopon, @EisenbergEffect it would be great to have a few reference use-cases for coerce so we can validate our implementation works for them. I'm worried coerce will cause a bunch of confusion due to the view and view-model values not matching and the odd scenarios that will result, similar to what happens when folks attempt to use a value converter or a property setter to make a number/date control. If you look through the binding issues folks have opened there are a bunch related to folks being surprised that the html input element's value property coerces to string. Here's a few:
#442
#617
aurelia/framework#288
#261
#443

bigopon · 2017-12-11T01:01:01Z

@EisenbergEffect Just rechecked it, it is working as expected, In the gist I wasn't declaring @observable.date / @bindable.date on both side of binding for <app/> <-> <date-picker/> , that's why it is not freezing. So my gotchas above are good. It's just the reference use-cases @jdanyow mentioned.

CasiOo · 2017-12-12T00:15:49Z

src/coerce-functions.js

+  },
+  number(a) {
+    const val = Number(a);
+    return !isNaN(val) && isFinite(val) ? val : 0;


I would expect either NaN or Infinity to be returned if not a valid number, not 0.

NaN doesn't work nicely with === (two way binding scenarios), to handle it we need to sacrifice perf for more checks. With hundreds of bindings perf hit will be worse. I'm not sure what to do here. Infinity, on the other hand, still problematic but could be treated as is.

I see it is a problem with two-way binding. Still, as a developer I would not expect 0 to be returned

I think it could be application specific where NaN / Infinity are expected. For that case, we can always define another decorator via createTypedObservable:

import { coerceFunctions, createTypedObservable } from 'aurelia-binding' coerceFunctions.$number = function(value) { return Number(value); }; createTypedObservable('$number');

// usage

class App { @observable.$number() numProp }

CasiOo · 2017-12-12T00:40:52Z

src/coerce-functions.js

+  boolean(a) {
+    return !!a;
+  },
+  date(val) {


Since dates are stateful, should there be a check if it's already of type date?
I haven't thought this through, just want to make sure this case has been considered.

if (typeof val === 'date') { return val; }

This could be nice. I didn't think of it. Maybe we can reuse the instance if this ever gets merged ? @jdanyow / @EisenbergEffect

gheoan · 2018-01-22T20:25:23Z

Aurelia does not provide any value converters so why would the coerce functions be provided?
All basic type constructors can be used as functions for type conversion. Why use @observable({ coerce: 'number' }) prop instead of @observable({ coerce: Number }) prop.
Is there any use case for the fluent syntax other then saving a few keystrokes?
Sorry to delay this PR any longer. I sugest splitting this PR, leaving one PR with only the basic functionality (the coerce attribute accepting a function) so that it can be merged faster.

bigopon · 2018-01-22T23:03:37Z

@gheoan Imo, it is good to have interface used for dependencies declaration. In JS, there is no interface so string is the best option. Plus there are unexpected behavior when you call primitive constructor directly, for example

Date(new Date()) !== new Date(new Date())

And how do you override existing / built in implementation if everything is a function ? It is sometime desired to do so. For example, in this PR implementation, we have identical behavior between:

class ABC {
  @observable({ coerce: 'string' }) value 
}

class ABC {
  @observable({ coerce: String }) value
}

because they both dont care null / undefined. It will just be converted as is. Now in someone's app, it's better to have type string, but null/ undefined to an empty string. Enforcing function would
require the converter function to be imported everywhere, which is what I don't believe we want to do, like following:

import { RealStringConverter } from './util'

class ABC {
  @observable({ coerce: RealStringConverter }) value
}

While with string:

// main.js
import { coerceFunctions } from 'aurelia-binding'
coerceFunctions.string = val => val === null || val === undefined ? '' : val.toString();

// app.js
class ABC {
  @observable({ coerce: 'string' }) value
}

Does the above answer your question ?

I do want to see the PR get merged, but like @jdanyow stated, it would be nice if we could have real world usage to validate the implementation. It works fine in tests but the experience could vary in app.

bigopon · 2018-01-22T23:11:12Z

@EisenbergEffect @jdanyow Maybe this could be enabled via plugin form, so people can try it out and before we include it in the box ?

jdanyow · 2018-03-30T04:06:19Z

agree- plugin would be better for something like this. I'm worried that it will be hard to support given all the issues mentioned in #623 (comment)

bigopon · 2018-03-31T04:22:23Z

For anyone interested in this feature: https://github.com/bigopon/aurelia-typed-observable-plugin

rek72-zz · 2018-04-16T16:09:56Z

Thanks @bigopon for the plugin. My only question is that if we use this, is there much concern over future aurelia binding releases? Are you all rewriting this or just extending the binding from aurelia?

feat(observable): coercion, typed observable

d06e24a

bigopon mentioned this pull request Sep 26, 2017

WIP feat(bindable): support coercing value aurelia/templating#558

Closed

3 tasks

handle coerce invalid date

3b2eb53

bigopon mentioned this pull request Oct 18, 2017

Aurelia should support boolean values for attributes. #347

Closed

add test for inheritance

d35935f

atsu85 reviewed Oct 19, 2017

View reviewed changes

use isNaN to check invalid date

ad49a94

usePropType -> usePropertyType

5724307

nashwaan mentioned this pull request Nov 2, 2017

Add a type option to bindable for type conversion aurelia/templating#96

Closed

CasiOo reviewed Dec 12, 2017

View reviewed changes

jdanyow closed this Mar 30, 2018

bigopon deleted the binding-typed-observable branch March 31, 2018 04:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(observable): coercion, typed observable #623

feat(observable): coercion, typed observable #623

bigopon commented Sep 26, 2017

jdanyow commented Sep 30, 2017

bigopon commented Oct 1, 2017 •

edited

Loading

EisenbergEffect commented Oct 16, 2017

bigopon commented Oct 16, 2017 •

edited

Loading

nashwaan commented Oct 19, 2017

bigopon commented Oct 19, 2017

atsu85 Oct 19, 2017

bigopon Oct 19, 2017

StrahilKazlachev Oct 19, 2017

bigopon Oct 19, 2017

atsu85 Oct 19, 2017 •

edited

Loading

bigopon Oct 19, 2017

bigopon Oct 19, 2017

bigopon commented Oct 20, 2017

bigopon commented Dec 10, 2017

EisenbergEffect commented Dec 10, 2017

jdanyow commented Dec 11, 2017

bigopon commented Dec 11, 2017 •

edited

Loading

CasiOo Dec 12, 2017

bigopon Dec 12, 2017 •

edited

Loading

CasiOo Dec 12, 2017

bigopon Dec 12, 2017

CasiOo Dec 12, 2017

bigopon Dec 12, 2017 •

edited

Loading

gheoan commented Jan 22, 2018

bigopon commented Jan 22, 2018

bigopon commented Jan 22, 2018

jdanyow commented Mar 30, 2018

bigopon commented Mar 31, 2018

rek72-zz commented Apr 16, 2018

feat(observable): coercion, typed observable #623

feat(observable): coercion, typed observable #623

Conversation

bigopon commented Sep 26, 2017

jdanyow commented Sep 30, 2017

bigopon commented Oct 1, 2017 • edited Loading

Gotchas

EisenbergEffect commented Oct 16, 2017

bigopon commented Oct 16, 2017 • edited Loading

nashwaan commented Oct 19, 2017

bigopon commented Oct 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

atsu85 Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigopon commented Oct 20, 2017

bigopon commented Dec 10, 2017

EisenbergEffect commented Dec 10, 2017

jdanyow commented Dec 11, 2017

bigopon commented Dec 11, 2017 • edited Loading

Choose a reason for hiding this comment

bigopon Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bigopon Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

gheoan commented Jan 22, 2018

bigopon commented Jan 22, 2018

bigopon commented Jan 22, 2018

jdanyow commented Mar 30, 2018

bigopon commented Mar 31, 2018

rek72-zz commented Apr 16, 2018

bigopon commented Oct 1, 2017 •

edited

Loading

bigopon commented Oct 16, 2017 •

edited

Loading

atsu85 Oct 19, 2017 •

edited

Loading

bigopon commented Dec 11, 2017 •

edited

Loading

bigopon Dec 12, 2017 •

edited

Loading

bigopon Dec 12, 2017 •

edited

Loading